🤖 Isaac Lab Review Bot

Summary

This PR fixes an issue where Newton model requirements were not correctly determined when environment configs were constructed in certain orders (e.g., cameras added in _setup_scene after scene cloning). The fix adds early registration of renderer requirements in Camera.__init__ and refreshes the visualizer clone function before cloning environments. It also reverts a previous fallback mechanism that built Newton models from USD traversal, which was slow and is no longer needed with the fix in place.

Architecture Impact

Cross-module impact is significant:

1. Camera.__init__ now calls SimulationContext.update_scene_data_requirements() — affects any code path that creates cameras before scene cloning
2. InteractiveScene.clone_environments() now re-evaluates requirements and updates the visualizer clone function — affects all environment cloning
3. PhysxSceneDataProvider loses its USD fallback mechanism — any code path that previously relied on this fallback will now fail if the prebuilt artifact is missing

The changes create a dependency where Camera sensors MUST be instantiated before clone_environments() is called for requirements to propagate correctly.

Implementation Verdict

Minor fixes needed — The core fix is sound, but there's a subtle ordering issue and missing error handling that could cause confusing failures.

Test Coverage

• Tests were updated to match the new behavior (removed fallback tests, removed xfail marks)
• Missing: No test verifies the new early-registration path in Camera._register_renderer_scene_data_requirements
• Missing: No test verifies _refresh_visualizer_clone_fn_from_requirements actually fixes the described issue
• The removed xfail marks on physx-warp-* tests suggest those now pass, which is good coverage for the fix

CI Status

No CI checks available yet.

Findings

🟡 Warning: source/isaaclab/isaaclab/sensors/camera/camera.py:158-177 — Silent failure when SimulationContext doesn't exist
The _register_renderer_scene_data_requirements method silently returns if SimulationContext.instance() is None. If a Camera is created before SimulationContext is initialized (which can happen in certain config-only workflows), the requirements won't be registered and the Newton model build will fail later with an obscure "missing artifact" error. Consider logging a debug message when this happens:

sim = SimulationContext.instance()
if sim is None:
    logger.debug("SimulationContext not available; deferring renderer requirements registration")
    return

🟡 Warning: source/isaaclab/isaaclab/scene/interactive_scene.py:257-275 — Duplicate requirements resolution logic
_refresh_visualizer_clone_fn_from_requirements duplicates logic from the __init__ method (lines 165-181). Both resolve requirements from sensor renderer types and update the simulation context. This creates maintenance burden and risk of divergence. Consider extracting to a shared helper method.

🔵 Improvement: source/isaaclab/isaaclab/scene/interactive_scene.py:232 — Unnecessary refresh when no sensors exist
_refresh_visualizer_clone_fn_from_requirements() is called unconditionally in clone_environments(), but if _sensor_renderer_types() returns an empty list (no sensors with renderer configs), the method does unnecessary work including requirements resolution and clone function resolution. Add an early return:

def _refresh_visualizer_clone_fn_from_requirements(self) -> None:
    renderer_types = self._sensor_renderer_types()
    if not renderer_types:
        return
    # ... rest of method

🔵 Improvement: source/isaaclab_physx/isaaclab_physx/scene_data_providers/physx_scene_data_provider.py:168-198 — Redundant artifact validation
The method checks not artifact on line 170, then separately checks model is None or state is None on line 180. The first check makes the second check redundant for the not artifact case. The error messages are also nearly identical. Consider consolidating:

if not artifact or artifact.model is None or artifact.state is None:
    self._last_newton_model_build_source = "missing"
    logger.error("[PhysxSceneDataProvider] Missing or incomplete visualizer prebuilt artifact...")
    self._clear_newton_model_state()
    return

🔵 Improvement: source/isaaclab/test/sim/test_physx_scene_data_provider_visualizer_contract.py — Missing test for new Camera registration path
The test file was updated to remove fallback tests but doesn't add coverage for the new Camera._register_renderer_scene_data_requirements method. Consider adding a test that verifies creating a Camera with a newton_warp renderer type updates the SimulationContext requirements:

def test_camera_registers_renderer_requirements():
    """Camera creation updates SimulationContext scene data requirements."""
    # Test that creating a Camera with renderer_cfg.renderer_type="newton_warp"
    # causes sim.update_scene_data_requirements to be called with newton requirements

🔵 Improvement: source/isaaclab_tasks/test/test_shadow_hand_vision_presets.py:329-330 — Removed xfail without explanation in test
The xfail marks for physx-warp-rgb and physx-warp-depth were removed, implying the tests now pass. This is correct behavior, but the reason ("PhysX tendon schemas unsupported by Newton ModelBuilder") was the original failure mode. A comment explaining why this now works would help future maintainers understand that the fix bypasses Newton ModelBuilder's USD traversal by using prebuilt artifacts instead.

🤖 Isaac Lab Review Bot

Summary

This PR fixes a bug where Newton model requirements were not correctly determined in some environment configurations (particularly Direct envs that add cameras in _setup_scene after scene construction). The fix refactors requirement resolution into a reusable _refresh_visualizer_clone_fn_from_requirements() method that aggregates discovered requirements with those already registered on the simulation context. Additionally, the PR reverts a previous fallback path in PhysxSceneDataProvider that built Newton models from USD traversal at runtime, which was slow and is no longer needed.

Architecture Impact

Moderate cross-module impact:

• InteractiveScene: Now calls _refresh_visualizer_clone_fn_from_requirements() in both __init__ and clone_environments(), ensuring requirements registered after scene init (e.g., by sensors/cameras) are captured.
• Camera: New _register_renderer_scene_data_requirements() method proactively registers renderer-driven requirements at camera construction time, before clone-time prebuild.
• PhysxSceneDataProvider: Removes the USD-traversal fallback (_build_newton_artifact_from_usd_fallback), making the provider strictly dependent on prebuilt artifacts from the cloner.
• The change to visualizer_cfg.py (default cam_source from "cfg" to "prim_path") affects all visualizers using the default config.

Implementation Verdict

Minor fixes needed — The core fix is sound and well-tested, but there are a few issues to address.

Test Coverage

Good test coverage for the new paths:

• test_interactive_scene.py: New test test_refresh_visualizer_clone_fn_uses_registered_requirements verifies the refactored method respects registered requirements.
• test_camera.py: New test test_camera_registers_renderer_scene_data_requirements verifies early registration.
• test_physx_scene_data_provider_visualizer_contract.py: Tests updated to reflect removal of USD fallback.

Missing: No integration test verifying the end-to-end scenario (Direct env with camera added in _setup_scene → Newton model correctly built). This was the original bug.

CI Status

Most CI jobs are pending or skipped. pre-commit and Build Wheel passed, which covers linting and packaging.

Findings

🔴 Critical: source/isaaclab_physx/isaaclab_physx/scene_data_providers/physx_scene_data_provider.py:165-199 — Removal of fallback creates silent failure mode for Direct envs

The PR removes _build_newton_artifact_from_usd_fallback() entirely, but the camera's early registration (the fix) only works when SimulationContext.instance() is available. For Direct envs that construct cameras before SimulationContext exists, _register_renderer_scene_data_requirements() silently returns (line 166-168 in camera.py), and the removed fallback was the safety net. This could break Direct envs that don't follow the expected initialization order.

# camera.py:166-168
sim = SimulationContext.instance()
if sim is None:
    logger.debug("SimulationContext not available; deferring renderer requirements registration.")
    return  # No retry mechanism exists

🟡 Warning: source/isaaclab/isaaclab/visualizers/visualizer_cfg.py:43 — Unrelated default change without migration guidance

The default cam_source changed from "cfg" to "prim_path". This is a behavior change affecting all visualizers using the default config. Users who relied on eye/lookat positioning will now get camera-prim following instead. This should be documented in release notes or reverted if unintentional.

🟡 Warning: source/isaaclab/isaaclab/scene/interactive_scene.py:234-261 — Double resolution in clone_environments when called after init

clone_environments() now calls _refresh_visualizer_clone_fn_from_requirements() unconditionally, but __init__ already calls it. When has_scene_cfg_entities is True, clone_environments is called from __init__, resulting in two consecutive calls. The second call is redundant but not harmful — just wasteful.

# Line 189-195 in __init__:
self._refresh_visualizer_clone_fn_from_requirements(requested_viz_types)

if has_scene_cfg_entities:
    self.clone_environments(...)  # Calls _refresh_visualizer_clone_fn_from_requirements() again

🟡 Warning: source/isaaclab/isaaclab/sensors/camera/camera.py:157-178 — ValueError silently caught and ignored

When requirement_for_renderer_type(renderer_type) raises ValueError for an unknown renderer type, the exception is caught and the method returns silently. This hides configuration errors from users.

try:
    renderer_req = requirement_for_renderer_type(renderer_type)
except ValueError:
    return  # Should this log a warning for unknown renderer types?

🔵 Improvement: source/isaaclab/test/scene/test_interactive_scene.py:135-186 — Test setup duplicates mock configuration

Both test_clone_environments_non_cfg_invokes_visualizer_clone_fn and test_refresh_visualizer_clone_fn_uses_registered_requirements set up similar mock scenes. Consider extracting a shared fixture or helper to reduce duplication and improve maintainability.

🔵 Improvement: source/isaaclab/isaaclab/scene/interactive_scene.py:237-261 — Consider caching resolved clone_fn to avoid recomputation

_refresh_visualizer_clone_fn_from_requirements() recomputes requirements and resolves the clone function on every call. If requirements haven't changed, this work is redundant. Consider short-circuiting when requirements == current_req and visualizer_clone_fn is already set.